fix: stop push notifications after session expires without explicit logout - #248

This guard reads IFirebaseService.apps, which is only populated after the unawaited initializeApp(...) in main.dart:37 — and on this particular path there is exactly one chance to get it right.

What's verifiable in the repo:

• main.dart:37 starts initializeApp() without await, and FirebaseService.initializeApp only does _apps.add(name) after await Firebase.initializeApp(...) (firebase_service.dart:34-35).
• RefreshListenable registers a permanent _ref.listen(loginProvider, ...) (refresh_listenable.dart:9) from inside Router.build(), which ThingsboardApp.build watches on the first frame. So the autoDispose notifier is created once and kept alive — Login.build(), and therefore the Future(() => handleUserLoaded()) at :38, runs exactly once per launch.
• handleUserLoaded has only two live call sites: that one-shot Future and the UserLoadedEvent listener.
• CommunicationService wraps a plain EventBus with no replay, so the UserLoadedEvent the client fires while clearing the expired token — inside setUpRootDependencies(), before runApp and before that listener exists — is dropped.

Put together: if apps is still empty when that single Future runs, the cleanup is skipped and nothing re-triggers it for the rest of the session — the fix silently no-ops on exactly the reported scenario.

What I can't tell from reading is whether Firebase actually loses that race. It's [native init started just before runApp] versus [one await AppLinks().getInitialLink() + runApp + first frame + one event-loop turn], which is device-dependent and probably fine most of the time — the isCustomEndpoint() check inside initializeApp is cheap since _cachedEndpoint is already warm from TbClientService.init(). So I'm not claiming this is broken.

But awaiting initializeApp() in main() costs nothing (it returns null rather than throwing on failure), removes the question entirely, and makes the three other apps.isNotEmpty guards deterministic too. Worth doing before merge — and worth one manual check on a genuinely cold start (app killed, token expired days ago) rather than with the app already running, since that's the only configuration where this single-shot path is exercised.

Separately, and much more minor: this is now the fourth copy of the getIt<IFirebaseService>().apps.isNotEmpty guard wrapped around a NotificationService call (Login.logout at :43, Login._onFullyLoggedIn at :119, TbContext.logout at :306, and here). It reads like an invariant of the service rather than something each call site should have to remember — which is another argument for pushing the check down into NotificationService.

Fixed in 04f3f07 on both fronts. main() now awaits initializeApp() (it swallows its own errors and returns null, so the surrounding try/catch behavior is unchanged), which removes the race on the single-shot path. And the apps.isNotEmpty guard moved into NotificationService itself — init(), logout() and the renamed cleanUpStalePushRegistration() early-return when Firebase isn't configured — so all four call-site copies are gone (Login.logout keeps only its isFullyAuthenticated() check). Verified manually on a genuinely cold start with an expired refresh token: the cleanup runs and pushes stop.

Awaiting here puts the push cleanup in front of state = const LoginState(isUserLoaded: false), which is what AuthRedirect keys off (auth_redirect.dart:36) to route to the login screen.

Having traced what the cold-start path actually does, most of it is cheap: _fcmToken is null on the freshly-constructed singleton so removeMobileSession is skipped entirely, the three subscription cancels are no-ops, and the TbStorage read hits an already-open in-memory Hive box (_tb_secure_storage.dart:36). The one that isn't cheap is _messaging.deleteToken(), which has to reach FCM to drop the registration — on a bad network that's what delays the login screen appearing. (On the "expired while the app was running" path removeMobileSession is in play too; whether that short-circuits locally instead of hitting the wire depends on client internals I couldn't check, since thingsboard_ce_client 4.4.0 isn't vendored here.)

Not dramatic, but nothing in handleUserLoaded needs the result and handleSessionExpired() already swallows its own errors, so setting the state first and then kicking off the cleanup would keep the login screen's latency independent of the network. Because the flag is only deleted at the very end of logout(), an interrupted cleanup already retries on the next launch — which is what makes fire-and-forget safe here.

Reordered in 04f3f07: the state is set first and the cleanup is now unawaited(...), so the login screen no longer waits on deleteToken(). The flag-deleted-last ordering that makes the retry safe is asserted by a test ("keeps the registration flag when the cleanup is interrupted"), and the airplane-mode cold start was checked manually — login screen appears immediately, cleanup completes on the next launch.

The rest of the app funnels persisted keys through DatabaseKeys (lib/constants/database_keys.dart) and reads/writes them behind ILocalDatabaseService, whose implementation is the only place the raw key strings and the as String? casts live (local_database_service.dart:15-46). This adds a fourth persisted key that bypasses both. NotificationsLocalService does own its own key, so there is precedent — but that one at least concentrates its storage access in a dedicated service.

Adding pushNotificationsRegistered to DatabaseKeys plus a small isPushRegistered/setPushRegistered/clearPushRegistered trio on ILocalDatabaseService would keep the convention and, as a bonus, give the test a trivially fakeable seam instead of needing a mocked TbStorage in the global locator.

Moved in 04f3f07: the key is DatabaseKeys.pushNotificationsRegistered and the storage access sits behind isPushRegistered()/setPushRegistered()/clearPushRegistered() on ILocalDatabaseService. NotificationService resolves it as a final ILocalDatabaseService _localDatabase = getIt(); field, and the tests mock that seam instead of TbStorage.

Two small things here. The class already has a _log field (:22), so this could use it rather than adding another inline locator lookup — the surrounding getIt<TbLogger>() calls in this method are pre-existing, but new lines don't have to copy them.

More substantively, debug feels too low for a swallowed remote-call failure. Since the endpoint is @PreAuthorize-guarded server-side, this catch will fire on every expired-session cleanup, so it's the normal path rather than an anomaly — but a removeMobileSession failing for some other reason (endpoint change, API regression) is exactly what you'd want visible in field logs. _log.warn would distinguish it from routine debug noise without crying wolf.

Changed in 04f3f07: the catch logs via _log.warn, and since the method was being restructured anyway, the pre-existing getIt<TbLogger>() lookups in logout() were switched to the _log field as well. The same warn level is used for the other previously fire-and-forget removeMobileSession failures (token-refresh listener, _resetToken).

The name promises more than the call site can actually establish: handleUserLoaded invokes this for any unauthenticated client — a fresh install that never logged in, the instant after an explicit logout, and (given Login.build() runs once per launch) every launch that starts logged out. It's harmless because the flag gates it, but someone reading the call site can't tell whether a session actually expired, and a future caller may reasonably assume the opposite — that it's only safe to call on genuine expiry.

Something like ensurePushUnregistered() or cleanUpStalePushRegistration() would describe what the method actually guarantees — idempotent, safe to call whenever we find ourselves unauthenticated — and make it hard to misuse. The doc comment is good; it's just doing work the name could do.

Renamed to cleanUpStalePushRegistration() in 04f3f07; the doc comment now states the actual guarantee — idempotent, safe to call whenever the client is unauthenticated.

The flag is now written from two sites reachable through three paths in this method — _saveToken() at :231 and :237, and this else — and _saveToken() is also reached from the onTokenRefresh listener at :64. Working out which combination of mobileInfo == null / token-age branches ends up marked takes a moment, and it's the kind of thing that quietly goes wrong when a branch is added.

Since every path here that leaves a usable registration ends with a non-null fcmToken, could this collapse to a single await _markPushRegistered() at the end of _getAndSaveToken() (guarded on the token being non-null)? You'd want to keep the call in _saveToken() for the :64 refresh path, so it's about dropping the duplication inside _getAndSaveToken rather than removing a site outright.

Collapsed in 04f3f07 to the shape you suggested: _getAndSaveToken() marks once at the end, guarded on a non-null token, and _saveToken() keeps its call for the refresh-listener path. The write is idempotent, so the overlap on the _saveToken paths is harmless.

Overriding the collaborator under test means these tests only cover the flag predicate — the behaviour this PR is actually about (deleting the local FCM token, deleting the flag, tolerating a failed removeMobileSession) is stubbed away, and the manual logoutCalls counter reimplements what mocktail's verify() already does.

As the repo's first test file this also sets the pattern for everything that follows, and "subclass the SUT to neuter its real method" stops working the moment the interesting logic lives inside the overridden method — which is already the case here. If logout() gets split into a server part and a _cleanupPushRegistration() part (see the comment on notification_service.dart:141), these tests could drive the real handleSessionExpired() against a mocked TbStorage/ThingsboardClient and assert the observable effects — verify(() => storage.deleteItem(...)), verify(() => userApi.removeMobileSession(...)) — with no subclass at all.

Rewritten in 04f3f07 with no subclass: FirebaseMessaging, the local-notifications plugin and the badge service are constructor-injectable with prod defaults unchanged (_messaging became a getter over FirebaseMessaging.instance, which also made the reassignment in _requestPermission unnecessary and keeps construction safe before Firebase init). The tests now drive the real cleanUpStalePushRegistration()/logout() and assert the observable effects — verify(() => messaging.deleteToken()), verify(() => localDatabase.clearPushRegistered()), verify(() => userApi.removeMobileSession(...)).

Every future service test in this repo will need this same locator bootstrap, and it's worth noting that two of the three registrations exist purely so the constructor's field initializers don't throw — TbLogger for _log, and ITbClientService with a MockThingsboardClient that's never asserted on, for _tbClient.

Since this is the first test file, pulling this into a shared test/helpers/ function — something like registerTestDependencies({TbStorage? storage}) with a matching getIt.reset() — would stop it being copy-pasted into the next dozen test files.

Extracted in 04f3f07 to test/helpers/test_dependencies.dart — registerTestDependencies({tbClient, localDatabase, firebaseService}) plus resetTestDependencies(), shared by both test files. The logger registers as a mock, so constructor field initializers are satisfied without console noise.

Stubbing with any() means the test passes regardless of which key is read, so a renamed or mistyped _pushRegisteredKey — or a second unrelated storage read added later — would go unnoticed. Matching the literal key (getItem('push_notifications_registered')) and adding a verify on it would make the test actually pin the persisted contract, which is the part most likely to drift.

The flag read moved behind ILocalDatabaseService, so the persisted contract is pinned one level down: local_database_service_test.dart asserts the literal 'push_notifications_registered' on setItem/containsKey/deleteItem against a mocked TbStorage — deliberately the string, not the constant, so a key rename fails the test instead of drifting.

Coverage gaps worth closing while the file is fresh:

1. The registered != 'true' branch is only exercised with null — a stored-but-different value, the case the string sentinel actually creates, is untested.
2. Nothing covers _markPushRegistered(), so neither _saveToken() nor the new fresh-token else in _getAndSaveToken() is verified to persist the flag — that's half the fix.
3. Nothing covers the flag deletion in logout() or the new removeMobileSession try/catch; both are stubbed out by the subclass.
4. The login_provider.dart side is untested — neither "unauthenticated triggers cleanup" nor "Firebase not configured skips it", and the latter is where the single-shot path I flagged at login_provider.dart:54 lives.

Since NotificationService is a plain class, one test that exercises the real handleSessionExpired() → cleanup path end to end against a mocked storage and client would cover most of these at once, rather than testing the branch predicate in isolation.

Mostly closed in 04f3f07: (1) the string sentinel no longer exists — the flag is a bool checked via containsKey; (2) flag persistence is pinned in the LocalDatabaseService tests; (3) logout()'s try/catch and the flag deletion run for real in the tests ("still cleans up when removeMobileSession fails"), plus an interrupted-cleanup-retries case. Still untested: the _getAndSaveToken wiring (needs built_value Response<MobileSessionInfo> stubbing — left out as low value for the fix) and the login_provider side; the single-shot risk there is gone now that main() awaits Firebase init, and the Firebase-not-configured skip is covered at the service level.

The extraction left the two callers with different error handling, and I think this side got the wrong one. cleanUpStalePushRegistration() wraps its call in try/catch at :166 — deliberately, per the comment and the "keeps the registration flag when the cleanup is interrupted" test — but here the teardown runs bare, so anything it throws propagates out of NotificationService.logout().

What that costs: Login.logout() awaits this and only then calls _tbClient.logout(...), so a failing _messaging.deleteToken() aborts the logout before the client session is cleared — the user taps Log out and stays logged in, and the exception surfaces unhandled at the call site (more_page.dart:96, profile_page.dart:58, update_required_handler.dart:19 and the rest all await it with no catch). Offline logout is exactly when deleteToken() fails, and it's the same failure the interrupted-cleanup test already exercises on the other path.

The inline version before this commit had the same exposure, so it isn't a regression — but now that the teardown is one named method with one caller already guarding it, moving the try/catch inside _cleanupPushRegistration() makes both paths uniform for a couple of lines. The flag-deleted-last ordering means a swallowed failure here retries on the next launch, same as the stale path, so the semantics stay consistent.

Fixed in cef25b4. The try/catch now lives inside the teardown itself (_tearDownLocalPushState(), the renamed _cleanupPushRegistration()), so logout() and cleanUpStalePushRegistration() share the same failure handling. clearPushRegistered() still runs last, so a failed offline logout leaves the flag set and the next launch retries the teardown. Pinned by NotificationService.logout completes when the local teardown fails (e.g. offline) and keeps the registration flag for the next launch.

This lands in dev_dependencies, but two production files already import it: lib/core/auth/noauth/provider/noauth_provider.dart:1 and lib/utils/services/tb_image_gallery_service/tb_image_gallery_service.dart:3. They were relying on it transitively (it isn't declared at all on develop/1.9.0), which is why nothing complained. Declaring it dev-only now states that shipped code doesn't use it, which isn't true — since it's being promoted out of transitive anyway, it belongs under dependencies.

Worth a glance at the constraint too: dio's types reach the app through thingsboard_ce_client's API — which is what the new test's Response<void> is standing in for — so pinning a range here independently of what the client pins is skew waiting to happen. pubspec.lock resolves to the same version today, so this is about the constraint rather than the current build.

Moved to dependencies in cef25b4; pubspec.lock now records it as direct main. Kept ^5.7.0, which is the same constraint thingsboard_ce_client pins, so the two cannot drift apart within the major.

Still unresolved: the registration half of the mechanism is untested. local_database_service_test.dart pins that LocalDatabaseService.setPushRegistered() writes the right key — that's the storage contract, one level below this finding. What's missing is anything asserting that NotificationService ever calls it: that _getAndSaveToken()'s branches (no server-side session, a session older than 30 days going through _resetToken, a fresh session) all persist the flag, and that a null FCM token doesn't. As it stands the flag could stop being written entirely and every test here would still pass, leaving the feature silently dead — which is the failure mode the original comment was about, and the branch shape at :290 is where it would happen.

On the stated cost: I went looking for a structural blocker and didn't find one. init() is reachable in a test — FirebaseMessaging.onMessage/onMessageOpenedApp are static, but they sit on plain static broadcast StreamControllers (firebase_messaging_platform_interface-4.6.10/lib/src/platform_interface/platform_interface_messaging.dart:74,86), so listening to them needs no Firebase app and doesn't throw. _initFlutterLocalNotificationsPlugin() only touches the injected plugin plus const constructors, and _getNotificationsCountRemote() swallows its own errors, so an unstubbed call there is harmless. The MobileSessionInfo for getMobileSession is built the same way production code already builds it (MobileSessionInfo((b) => b..fcmTokenTimestamp = ...)). What's actually needed is a NotificationSettings fixture for requestPermission — a const constructor with about a dozen required params, so one helper. Real work, but a moderate cost rather than a blocker.

The login_provider gap is also still open — see the comment on login_provider.dart:56, where the interleaving question gives it a second reason to exist. And while the Firebase-not-configured skip is tested for logout() and cleanUpStalePushRegistration(), init()'s copy of the same early return isn't.

Added in cef25b4: init() now runs for real in the tests and drives _getAndSaveToken() through all three branches — no server-side session, fresh server-side session, and the 30-day rotation via _resetToken() — each asserting setPushRegistered() exactly once. A null FCM token and a denied permission assert it is never written, and the Firebase-not-configured early return of init() is covered too. NotificationSettings is built by a small permission() helper in the test file, as you predicted.

The login_provider wiring test is still deferred: it needs a Riverpod container plus IDeviceInfoService/IOverlayService/ICommunicationService in test/helpers, which is a bigger lift than this PR.

This early return predates the rewrite, but the whole handler is new code now, so it's worth a second look: when there's no previous token the refresh event is dropped entirely, including the _saveToken(token) that would register the device.

That state is reachable. getToken() returns null when _messaging.getToken() throws (offline), _getAndSaveToken() then returns early at :268 leaving _fcmToken unset — and the listener is still installed here, because the early return doesn't throw. When connectivity comes back and FCM emits a token, this drops it, so the device stays unregistered for the rest of the session and the flag is never written. It self-heals on the next launch, so it's not severe, but the fix is small: skip only the removeMobileSession block when _fcmToken is null and always fall through to _saveToken(token). Was the current shape intentional, i.e. "no previous session means nothing to do"?

Worth a test while you're in here — unlike the static message streams, onTokenRefresh is an instance getter (firebase_messaging-15.2.10/lib/src/messaging.dart:126), so a StreamController on the injected mock drives this handler directly: emit a token, assert the old session is removed and the new one saved. It's the most intricate branch in the service (sequential awaits, _fcmToken swapped before the remote calls, two independent error swallows) and currently has no coverage at all.

Not intentional — the pre-PR handler had the same shape (if (_fcmToken != null) { removeMobileSession(...).then(save) }) and I carried it over when converting it to awaits. In cef25b4 only the removeMobileSession block is skipped when there was no previous token; _saveToken(token) always runs.

Three tests now drive the handler through a StreamController on the injected mock: the normal move of the session to the refreshed token, a refresh arriving after the initial getToken() failed (no removal, save + flag), and a refresh where removing the previous session fails (save still happens).

Making this fire-and-forget fixes the latency, but it also means the cleanup can now overlap init() on the same NotificationService singleton, which the awaited version couldn't.

The interleaving: this starts _cleanupPushRegistration(), which blocks on _messaging.deleteToken() (the one genuinely slow step). If the client authenticates while that's still pending, it fires UserLoadedEvent → the listener at :34 → handleUserLoaded() → _onFullyLoggedIn() → NotificationService.init(), which acquires a token, saveMobileSessions it, sets the flag and calls setAutoInitEnabled(true). The cleanup then resumes against that new state: _fcmToken = null, setAutoInitEnabled(false), clearPushRegistered() — and if getToken() resolved before the pending deleteToken() landed, the token that was just registered is the one that gets deleted. I checked the server side of that outcome: MobileAppNotificationChannel.java:122-128 only purges the orphaned session when a push actually bounces, and nothing re-registers from the platform side, so the recovery is the next init() — i.e. no push for the rest of that session.

On how reachable it is, I'd calibrate this lower than it first looks. The window is however long deleteToken() takes, and init() reaches getToken() only after getInitialMessage() and requestPermission(). A human typing credentials (≥1-2s) loses that race comfortably, so the ordinary login path is fine. The cases where both sides are network-paced rather than human-paced are the ones to think about: a QR/deep-link launch that lands back on the default endpoint (noauth_provider's reset() → _initDefaultFbApp() re-populates apps, then the client logs in without user input) and returning from OAuth2. On a custom endpoint there's no race at all — both sides early-return on _isFirebaseConfigured.

So: an edge case rather than a blocker, but the fix is about three lines — hold the future in the service and await it at the top of init(). That also covers a smaller version of the same thing: every UserLoadedEvent arriving unauthenticated fires another unawaited(...), and since the flag is only cleared at the end, concurrent calls all pass the isPushRegistered() check and run the teardown in parallel.

Separately, and reasonable to defer: this file is where the fix actually manifests, and none of it is covered — every test targets NotificationService in isolation. A provider-level test (unauthenticated mock client → cleanup ran; authenticated → init() ran instead) is what would catch a future refactor dropping this call, though it does need a Riverpod container plus IDeviceInfoService/IOverlayService/ICommunicationService added to test/helpers, so it's a bigger lift than the service-level tests.

Fixed in cef25b4, in the service rather than the provider: cleanUpStalePushRegistration() stores its future in _staleCleanup (shared by concurrent callers, reset in whenComplete), and init() awaits that future right after the Firebase guard, before touching the token. That also covers the smaller version — repeated unauthenticated UserLoadedEvents now share one teardown instead of running several in parallel.

Tests: waits for a pending stale cleanup, so the token it registers is not the one being deleted holds deleteToken() on a Completer, asserts getToken() has not been called while it is pending, then checks the clearPushRegistered → getToken → setPushRegistered order; runs a single teardown for concurrent calls covers the dedup. The provider-level test is deferred as you suggested.

Optional. Collapsing the three branch-local calls into one is a real improvement, but _saveToken() still calls setPushRegistered() at :302, so on the mobileInfo == null and stale-token paths the flag is written twice back to back — two of the three paths. Idempotent, as you noted, so this is about clarity rather than behaviour: it leaves the invariant without an owner, and the next person adding a branch has to work out whether marking is _saveToken's job or the caller's.

Since this trailing block exists only for the case where mobileInfo is fresh and no save happens, putting an explicit setPushRegistered() in that one branch and dropping the trailing call would give each path a single owner.

Restructured _getAndSaveToken() with early returns in cef25b4: _saveToken() owns the flag write on the two paths that save a session, and the fresh-session branch calls setPushRegistered() explicitly. The trailing block is gone, so each path writes the flag exactly once — the three init() tests assert called(1).

Optional. isPushRegistered() reads through containsKey, so the true written here is never read back — the flag's truth is key presence and the stored value is decoration. Presence-as-flag is a fine pattern, but it's a quiet trap: someone who later needs a false state will write setItem(key, false) and it will still read as registered.

A line of comment saying presence is the contract is probably the right-sized fix; reading the value (await storage.getItem(...) == true) is the alternative if you'd rather the payload be the thing that matters.

Went with reading the value in cef25b4: isPushRegistered() is now await storage.getItem(key) == true, so a stored false reads as not registered. local_database_service_test.dart covers getItem returning true and null.

Optional, but it's now-or-never: the three keys above persist camelCase strings (thingsBoardApiEndpoint, initialAppLink, selectedRegion) and this one persists snake_case. There's no data in the wild yet, so 'pushNotificationsRegistered' would line it up with its siblings at zero cost beyond updating the literal in local_database_service_test.dart. Purely cosmetic — the only time it surfaces is someone inspecting the Hive box — so equally fine to leave as is if you'd rather not touch the test.

Renamed to 'pushNotificationsRegistered' in cef25b4; the literal in local_database_service_test.dart is updated.

One suggestion worth taking and one to weigh.

Worth taking: addTearDown(getIt.reset) inside this function would make the cleanup impossible to forget. Right now every main() has to remember to wire resetTestDependencies into its own tearDown, and a file that forgets will leak registrations into the next one and fail in a confusing way. Since this helper is about to be used by every service test in the repo, moving that responsibility here is cheap insurance.

To weigh: the nullable-with-default-mock params don't quite work as an API — a caller that omits localDatabase gets a mock it has no handle on, so it can neither stub nor verify it, and both current callers pass everything explicitly anyway. Making the params required, or returning what was created, would make the default path usable. Lower value than the tear-down point, and fine to leave until a second caller actually wants it.

Both taken in cef25b4: registerTestDependencies() calls addTearDown(getIt.reset) itself (resetTestDependencies and the per-file tearDown are gone), and the three params are required.

Optional. This four-line arrange — stub getToken, stub removeMobileSession, build the service, await service.getToken() to prime _fcmToken — is repeated across three of the four logout tests with only the removeMobileSession behaviour differing. A Future<NotificationService> buildServiceWithToken() next to buildService() would drop the repetition and, more usefully, put a name on the non-obvious "you have to call getToken() first to populate the private field" ritual.

Added buildServiceWithToken() in cef25b4, with a comment on why getToken() has to run first; the logout tests that need a primed token use it.

This read sits outside the swallow. _tearDownLocalPushState() catches everything, but isPushRegistered() runs before it, so a storage failure completes _staleCleanup with an error instead of logging it.

I probed it rather than assuming: stubbing isPushRegistered() to throw makes cleanUpStalePushRegistration() complete with that error, and a following init() rethrows it and never reaches setPushRegistered() — so push registration is skipped for the whole session, and nothing catches it on the way up (handleUserLoaded() is driven from a stream listener and a bare Future(...)).

On likelihood I'd calibrate this low. secureStorage.init() is awaited in setUpRootDependencies() (locator.dart:38) before the app runs, so the late box is always open by the time the login provider builds — a throw needs box corruption or a decryption failure, not an ordinary boot.

The reason I'd still move the line is narrower than the crash risk: two things currently assert this can't happen. The comment at login_provider.dart:53-55 says the cleanup swallows its own errors, and init() awaits the future as if it can't fail. Both are true of the teardown and false of this one line. Pulling the check inside the try makes them true and doesn't change the retry semantics, since the flag is untouched when the read fails.

Fixed in d7496b3. _tearDownIfRegistered() now reads the flag inside its own try/catch: a failing read logs a warning and returns, leaving the flag untouched, so the shared future can never complete with an error and init() awaits it safely. The doc comment now says "never throws". Pinned by completes when the registration flag cannot be read, so a later init() still registers, which is your probe turned into a test: isPushRegistered() stubbed to throw, then init() still reaches setPushRegistered().

The dedup and the init() handshake both work, and the two new tests earn their place. Recording an asymmetry for the doc comment rather than asking for a change:

The guard is one-directional. It stops a cleanup already in flight from being overtaken by init(), but nothing stops a cleanup that starts while init() is mid-flight, and logout() funnels into the same teardown while ignoring _staleCleanup. I probed the open direction by parking init() on requestPermission() and firing the cleanup while it was suspended: the call order comes out setPushRegistered() -> clearPushRegistered(), i.e. the server-side session is saved and then the local token is deleted and the flag cleared.

I then checked how bad that end state actually is, against the platform source, and it is milder than it first looks:

• MobileAppNotificationChannel removes the mobile session when a push comes back UNREGISTERED, so the orphaned server-side session is purged on the next delivery attempt.
• The next app launch mints a fresh token (the old one was deleted), getMobileSession returns null for it, and _saveToken() re-registers and re-sets the flag. So it self-heals completely without user action.

Net impact is therefore "no pushes for the remainder of the current session", on a trigger that needs an unauthenticated UserLoadedEvent to land inside init()'s window. I'd leave the code as it is — reworking the service's concurrency model (one chained future that every entry point extends) to close a self-healing single-session gap isn't proportionate, and it would add machinery to three methods that currently read clearly.

The one thing worth doing is a word in the doc comment. "Concurrent calls share a single run, and [init] waits for it to finish" reads as though the ordering is fully established in both directions; saying it guards the cleanup-then-init direction would stop the next reader relying on more than is there.

Minor separate nit on this line: dropping async to return Future.value() reads oddly beside the rest of the class. An async body still runs synchronously up to its first await, so async with a plain return; would keep the dedup intact and stay consistent.

Doc comment updated in d7496b3: it now says init() waits for a run that is already in flight, that the reverse direction is not ordered, and what that costs (pushes until the next launch registers a fresh token). Code left as is, per your recommendation.

Also took the nit: the method is async again with a plain return;; the ??= still runs synchronously before the first await, and runs a single teardown for concurrent calls keeps passing.

Optional, and weaker than I first thought. Consolidating the two catches was right; the message no longer names the flow, but I'd talked myself into overrating that: both callers already log distinctly immediately before the teardown (:136 for logout(), :177 for the stale path), so the flow is identifiable from the surrounding log stream.

What's genuinely gone is which of the eight steps failed — and that matters a little, because a failed deleteToken() means delivery continues and the retry on next launch is the point, whereas a failed cancelAll() is cosmetic and the retry achieves nothing. In practice $e usually names the operation anyway. Worth a step name only if you're already touching this line.

Leaving this one. Naming the step means either a try/catch per await or a mutable step variable threaded through eight calls, and as you note the exception text usually names the failing operation already (FirebaseException carries the plugin method, and both callers log the flow right before). Not worth the extra machinery in a method that reads cleanly now.

This names the public caller rather than the method it's in, so it'll drift if _tearDownIfRegistered() ever gets a second caller or the public method is renamed — and the rest of the class follows Class::method() faithfully. Either move it up into cleanUpStalePushRegistration() (before the flag check, which would also log the no-op case) or name this method.

Fixed in d7496b3: the log line is NotificationService::_tearDownIfRegistered(), matching the Class::method() convention of the rest of the file.

Not re-opening this — your costing was right, and a Riverpod container plus three more service mocks is more than this PR should carry.

Noting it for whoever picks up the follow-up, because the valuable assertion is narrower than a full provider test: it isn't the whole login flow, it's the branch choice. Unauthenticated -> cleanup started; authenticated -> init() and no teardown. The second half is the one that would catch a future refactor moving this call above the isAuthenticated() check, where it would tear down the registration a normal login is about to create — and every existing test would still pass.

One assumption worth writing down while it's fresh: the ordering claim in the comment you added holds only because NotificationService is a lazy singleton in the locator. Registered as a factory, init() would await a different instance's _staleCleanup and the guarantee would quietly disappear.

Agreed on the shape of the follow-up test — the branch choice, not the flow. Recorded the singleton assumption in d7496b3 as a comment next to the await _staleCleanup in init(), so a future move of NotificationService to a factory registration has a comment to trip over.

The rotation path is covered for the happy case where _resetToken() hands back a fresh token, but not for the other side of if (freshToken != null) — rotation deleted the old token and then getToken() came back null, which is what happens if connectivity drops in between.

That's the more interesting half: the device ends up with no token, no saved session and no setPushRegistered(), so push is silently off until the next init(). Stubbing the second getToken() to null and asserting neither saveMobileSession nor setPushRegistered is called would cover it in a few lines, and it sits right next to the case you already have.